Re: Mask bit support's API

["Yang, Sheng" <sheng.yang@intel.com>]

On Tuesday 23 November 2010 20:47:33 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.

I am not sure about it. But I suppose the structure should be the same? In fact 
it's pretty hard for me to image what's needed for virtio in the future, 
especially there is no such code now. I really prefer to deal with assigned device 
and virtio separately, which would make the work much easier. But seems you won't 
agree on that.

> 
> > >  >  >   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.

This can be implemented by this API, just adding a flag for it. And I would still 
take this into consideration in the next API purposal.
 
> > >  >  >   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

I suppose we don't need to generate notification to userspace? Because every 
read/write is handled by kernel, and userspace just need interface to kernel to 
get/set the entry - and well, does userspace need to do it when kernel can handle 
all of them? Maybe not...

--
regards
Yang, Sheng