On Tuesday 23 November 2010 14:17:28 Avi Kivity wrote:
> On 11/23/2010 08:09 AM, Yang, Sheng wrote:
> > Hi Avi,
> >
> > I've purposed the following API for mask bit support.
> >
> > The main point is, QEmu can know which entries are enabled(by
> > pci_enable_msix()). And for enabled entries, kernel own it, including
> > MSI data/address and mask bit(routing table and mask bitmap). QEmu
> > should use KVM_GET_MSIX_ENTRY ioctl to get them(and it can sync with
> > them if it want to do so).
> >
> > Before entries are enabled, QEmu can still use it's own MSI table(because
> > we didn't contain these kind of information in kernel, and it's
> > unnecessary for kernel).
> >
> > The KVM_MSIX_FLAG_ENTRY flag would be clear if QEmu want to query one
> > entry didn't exist in kernel - or we can simply return -EINVAL for it.
> >
> > I suppose it would be rare for QEmu to use this interface to get the
> > context of entry(the only case I think is when MSI-X disable and QEmu
> > need to sync the context), so performance should not be an issue.
> >
> > What's your opinion?
> >
> > > #define KVM_GET_MSIX_ENTRY _IOWR(KVMIO, 0x7d, struct
> > > kvm_msix_entry)
>
> Need SET_MSIX_ENTRY for live migration as well.
Current we don't support LM with VT-d...
>
> > > #define KVM_UPDATE_MSIX_MMIO _IOW(KVMIO, 0x7e, struct
> > > kvm_msix_mmio)
> > >
> > > #define KVM_MSIX_TYPE_ASSIGNED_DEV 1
> > >
> > > #define KVM_MSIX_FLAG_MASKBIT (1<< 0)
> > > #define KVM_MSIX_FLAG_QUERY_MASKBIT (1<< 0)
> > > #define KVM_MSIX_FLAG_ENTRY (1<< 1)
> > > #define KVM_MSIX_FLAG_QUERY_ENTRY (1<< 1)
>
> Why is there a need for the flag? If we simply get/set entire entries,
> that includes the mask bits?
We still want QEmu to cover a part of entries which hasn't been enabled yet(which
won't existed in routing table), but kernel would cover all mask bit regardless of
if it's enabled. So QEmu can query any entry to check the maskbit, but not
address/data.
> What about the pending bits?
We didn't cover it here - and it's in another MMIO space(PBA). Of course we can
add more flags for it later.
>
> > > struct kvm_msix_entry {
> > >
> > > __u32 id;
> > > __u32 type;
> > > __u32 entry; /* The index of entry in the MSI-X table */
> > > __u32 flags;
> > > __u32 query_flags;
> > > union {
> > >
> > > struct {
> > >
> > > __u32 addr_lo;
> > > __u32 addr_hi;
> > > __u32 data;
>
> Isn't the mask bit in the last word? Or maybe I'm confused about the
> format.
I separated the entry and mask bit as I said above.
>
> > > } msi_entry;
> > > __u32 reserved[12];
> > >
> > > };
> > >
> > > };
> > >
> > > #define KVM_MSIX_MMIO_FLAG_REGISTER (1<< 0)
> > > #define KVM_MSIX_MMIO_FLAG_UNREGISTER (1<< 1)
> > > #define KVM_MSIX_MMIO_FLAG_MASK 0x3
> > >
> > > struct kvm_msix_mmio {
> > >
> > > __u32 id;
> > > __u32 type;
> > > __u64 base_addr;
> > > __u32 max_entries_nr;
> > > __u32 flags;
> > > __u32 reserved[6];
> > >
> > > };
>
> Also need a new exit reason to tell userspace that an msix entry has
> changed, so userspace can update mappings.
I think we don't need it. Whenever userspace want to get one mapping which is an
enabled MSI-X entry, it can check it with the API above(which is quite rare,
because kernel would handle all of them when guest is accessing them). If it's a
disabled entry, the context inside userspace MMIO record is the correct one(and
only one). The only place I think QEmu need to sync is when MSI-X is about to
disabled, QEmu need to update it's own MMIO record.
--
regards
Yang, Sheng
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
