Re: Mask bit support's API

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 23, 2010 at 02:47:33PM +0200, Avi Kivity wrote:
> On 11/23/2010 10:30 AM, Yang, Sheng wrote:
> >On Tuesday 23 November 2010 15:54:40 Avi Kivity wrote:
> >>  On 11/23/2010 08:35 AM, Yang, Sheng wrote:
> >>  >  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...
> >>
> >>  Isn't this work useful for virtio as well?
> >
> >Yeah, but won't be included in this patchset.
> 
> What API changes are needed?  I'd like to see the complete API.
> 
> >>  >  >   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.
> >>
> >>  When an entry is masked, we need to set the pending bit for it
> >>  somewhere.  I guess this is broken in the existing code (without your
> >>  patches)?
> >
> >Even with my patch, we didn't support the pending bit. It would always return 0
> >now. What we supposed to do(after my patch checked in) is to check IRQ_PENDING flag
> >of irq_desc->status(if the entry is masked), and return the result to userspace.
> >
> >That would involve some core change, like to export irq_to_desc(). I don't think
> >it would be accepted soon, so would push mask bit first.
> 
> The API needs to be compatible with the pending bit, even if we
> don't implement it now.  I want to reduce the rate of API changes.
> 
> >>
> >>  >  >   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.
> >>
> >>  So in-kernel handling of mmio would be decided per entry?  I'm trying to
> >>  simplify this, and simplest thing is - all or nothing.
> >
> >So you would like to handle all MSI-X MMIO in kernel?
> 
> Yes.  Writes to address or data would be handled by:
> - recording it into the shadow msix table
> - notifying userspace that msix entry x changed
> Reads would be handled in kernel from the shadow msix table.
> 
> So instead of
> 
> - guest reads/writes msix
> - kvm filters mmio, implements some, passes others to userspace
> 
> we have
> 
> - guest reads/writes msix
> - kvm implements all
> - some writes generate an additional notification to userspace

One small proposal in addition: since all accesses are done from guest
anyway, the shadow table can/should be stored using userspace memory,
reducing the kernel memory overhead of the feature from up to 4K per
MSIX table to just 8 bytes.

Active entries can be cached in kernel memory.

> 
> -- 
> error compiling committee.c: too many arguments to function