From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps Date: Tue, 11 Nov 2014 14:32:52 +0800 Message-ID: <5461AD94.2070008@intel.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.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> <545CB64E02000078000459CD@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <545CB64E02000078000459CD@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich 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 2014/11/7 19:08, Jan Beulich wrote: >>>> 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. Okay I will try to go there. > >> --- 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. Do you mean this line? copy_from_guest_offset(pcidevs, xdsr->pcidevs, 0, xdsr->num_pcidevs*sizeof(xen_guest_pcidev_info_t)) > >> --- 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. How about bool_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 Yes. Actually I ever considered this point but I just think we may need to keep a complete set of fields. You're right and anywhere what we should do is focusing on on-demand. > 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. Okay I can use 'force' directly. In Xen side we have 'bool_t', but we have 'bool' in tools side. So how to define this in xen/include/public/domctl.h? > >> +struct xen_domctl_set_rdm { >> + uint32_t pci_rdmforce; > > Same comment as on the field added to "struct hvm_domain". Ditto. > >> + 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. Looks I should do this, XEN_GUEST_HANDLE_64(xen_guest_pcidev_info_t) pcidevs; > >> @@ -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? As I understand we should shrink this pad when we introduce new filed, shouldn't we? Thanks Tiejun