From: "Jan Beulich" Subject: Re: [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps Date: Fri, 07 Nov 2014 11:08:46 +0000 Message-ID: <545CB64E02000078000459CD@mail.emea.novell.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <544F49F9.3070208@intel.com> <544F78B80200007800042B95@mail.emea.novell.com> <54509A8A.9060606@intel.com> <5450BE27020000780004304A@mail.emea.novell.com> <5451AC56.7010303@intel.com> <54521100020000780004363A@mail.emea.novell.com> <545320F2.5030103@intel.com> <545354500200007800043D94@mail.emea.novell.com> <5457174C.8020400@intel.com> <5457515102000078000443B0@mail.emea.novell.com> <54574D8F.8060407@intel.com> <54575E2D0200007800044443@mail.emea.novell.com> <545767C4.7070806@intel.com> <5457787002000078000445C7@mail.emea.novell.com> <54576DF7.8060408@intel.com> <545784830200007800044627@mail.emea.novell.com> <54585EAA.20904@intel.com> <545894610200007800044A5B@mail.emea.novell.com> <545992A2.8070309@intel.com> <545A57AD02000078000C1037@mail.emea.novell.com> <545B3F4A.5070808@intel.com> <545B562F02000078000453FB@mail.emea.novell.com> <545C9E97.4040800@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <545C9E97.4040800@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tiejun Chen Cc: yang.z.zhang@intel.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 07.11.14 at 11:27, wrote: > Are you saying Xen restrict some BDFs specific to emulate some devices? > But I don't see these associated codes. I didn't say so. All I said that some of the SBDFs are being used by them. >>> --- a/xen/include/xen/iommu.h >>> +++ b/xen/include/xen/iommu.h >>> @@ -158,14 +158,14 @@ struct iommu_ops { >>> void (*crash_shutdown)(void); >>> void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); >>> void (*iotlb_flush_all)(struct domain *d); >>> - int (*get_reserved_device_memory)(iommu_grdm_t *, void *); >>> + int (*get_reserved_device_memory)(iommu_grdm_t *, struct domain *, void *); >>> void (*dump_p2m_table)(struct domain *d); >>> }; >>> >>> void iommu_suspend(void); >>> void iommu_resume(void); >>> void iommu_crash_shutdown(void); >>> -int iommu_get_reserved_device_memory(iommu_grdm_t *, void *); >>> +int iommu_get_reserved_device_memory(iommu_grdm_t *, struct domain *, void *); >> >> I don't see why these generic interfaces would need to change; >> the only thing that would seem to need changing is the callback >> function (and of course the context passed to it). > > I'm not 100% sure if we can call current->domain in all scenarios. If > you can help me confirm this I'd really like to remove this change :) > Now I assume this should be true as follows: Which is wrong, and not what I said. Instead you should pass the domain as part of the context that gets made available to the callback function. > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1540,6 +1540,34 @@ int iommu_do_pci_domctl( > } > break; > > + case XEN_DOMCTL_set_rdm: > + { > + struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm; > + struct xen_guest_pcidev_info *pcidevs; > + int i; > + > + pcidevs = xmalloc_array(xen_guest_pcidev_info_t, > + domctl->u.set_rdm.num_pcidevs); > + if ( pcidevs == NULL ) > + { > + return -ENOMEM; > + } > + > + for ( i = 0; i < xdsr->num_pcidevs; ++i ) > + { > + if ( __copy_from_guest_offset(pcidevs, xdsr->pcidevs, i, 1) ) > + { > + xfree(pcidevs); > + return -EFAULT; > + } > + } I don't see the need for a loop here. And you definitely can't use the double-underscore-prefixed variant the way you do. > --- a/xen/include/asm-x86/hvm/domain.h > +++ b/xen/include/asm-x86/hvm/domain.h > @@ -90,6 +90,10 @@ struct hvm_domain { > /* Cached CF8 for guest PCI config cycles */ > uint32_t pci_cf8; > > + uint32_t pci_rdmforce; I still don't see why this is a uint32_t. > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -484,6 +484,24 @@ struct xen_domctl_assign_device { > typedef struct xen_domctl_assign_device xen_domctl_assign_device_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_assign_device_t); > > +struct xen_guest_pcidev_info { > + uint8_t func; > + uint8_t dev; > + uint8_t bus; > + int domain; > + int rdmforce; > +}; Please see struct physdev_pci_device_add for how to properly and space efficiently express what you need. And of course I'd expect the code to actually use all fields you specify here - neither domain (which really ought to be named segment if it is what I think it is meant to be) nor rdmforce are being used anywhere. Plus - again - just "force" would be sufficient as a name, provided the field is needed at all. > +struct xen_domctl_set_rdm { > + uint32_t pci_rdmforce; Same comment as on the field added to "struct hvm_domain". > + uint32_t num_pcidevs; > + XEN_GUEST_HANDLE(xen_guest_pcidev_info_t) pcidevs; Did you _at all_ look at any of the other domctls when adding this? There's not a single use of XEN_GUEST_HANDLE() in the whole file. > @@ -1118,7 +1137,8 @@ struct xen_domctl { > struct xen_domctl_gdbsx_domstatus gdbsx_domstatus; > struct xen_domctl_vnuma vnuma; > struct xen_domctl_psr_cmt_op psr_cmt_op; > - uint8_t pad[128]; > + struct xen_domctl_set_rdm set_rdm; > + uint8_t pad[112]; Why are you altering the padding size here? Jan