From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings Date: Wed, 27 Aug 2014 10:47:14 +0800 Message-ID: <53FD46B2.9010504@intel.com> References: <1408702186-24432-1-git-send-email-tiejun.chen@intel.com> <1408702186-24432-5-git-send-email-tiejun.chen@intel.com> <1409085374.28009.37.camel@citrix.com> <53FD3891.5020202@intel.com> <1409106035.28009.84.camel@citrix.com> <53FD4513.1020304@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FD4513.1020304@intel.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: Ian Campbell Cc: kevin.tian@intel.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, JBeulich@suse.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/8/27 10:40, Chen, Tiejun wrote: > On 2014/8/27 10:20, Ian Campbell wrote: >> On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote: >>> On 2014/8/27 4:36, Ian Campbell wrote: >>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote: >>>> >>>>> + /* We should check if mmio range is out of RMRR mapping. >>>>> + * >>>>> + * Assume we have one entry if not enough we'll expand. >>>>> + */ >>>> >>>> The usual approach with such hypervisor interfaces (which I suppose >>>> xc_reserved_device_memory_map turns into) is to first call it with NULL >>>> to get the required size and then allocate a suitable buffer and call a >>>> second time. >>> >>> Ofentimes, RMRR should be rare with one or two entries, even zero. >> >> It's not clear to me what number you are saying is the norm here. > > In my broadwell platform we just have two entries. > >> >> Even if some N is common today what guarantee is there that N won't grow >> or shrink with the next generation of systems? > > As I understand RMRR may be legacy, and this should go away. > > What is RMRR? > ------------- > > There are some devices the BIOS controls, for e.g USB devices to perform > PS2 emulation. The regions of memory used for these devices are marked > reserved in the e820 map. When we turn on DMA translation, DMA to those > regions will fail. Hence BIOS uses RMRR to specify these regions along > with devices that need to access these regions. OS is expected to setup > unity mappings for these regions for these devices to access these regions. > >> >>> So I >>> think its reasonable to start posting one entry since this can cover >>> such a scenario the platform really owns one entry. >> >> Making the call twice is not terribly expensive (nor is this a hot path) >> and it allows you to avoid the reallocation and recall and the twisty >> error handling structure which that implies. >> > > As I understand when we call one given hypercall, we may know that > possible numbers to issue that. Then we can get the appropriate number > via the returned value if that is not enough. I think its better than we > always issue twice hypercall unconditionally :) > > But if you persist in this fixed twice-call mechanism, I can try to > rework out this implementation :) > And actually, I think current implementation is already as you expect. Please check patch #3, @@ -4842,6 +4843,55 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) return rc; } + case XENMEM_reserved_device_memory_map: + { + struct xen_mem_reserved_device_memory_map map; + XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer; + XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param; + unsigned int i = 0; + struct xen_mem_reserved_device_memory rmrr_map; + struct acpi_rmrr_unit *rmrr; + + if ( !acpi_rmrr_unit_entries ) + return -ENOENT; + + if ( copy_from_guest(&map, arg, 1) ) + return -EFAULT; + + if ( map.nr_entries < acpi_rmrr_unit_entries ) + { + map.nr_entries = acpi_rmrr_unit_entries; + if ( copy_to_guest(arg, &map, 1) ) + return -EFAULT; + return -ENOBUFS; + } And turn back, here I just set map.nr_entries = 1, not '0' as you image. + /* Assume we have one entry if not enough we'll expand.*/ + uint32_t nr_entries = 1; + + /* We should check if mmio range is out of RMRR mapping. */ + if ( (map = malloc(nr_entries * + sizeof(xen_mem_reserved_device_memory_t))) == NULL ) So you really hope I should set map.nr_entries = 0 firstly? Thanks Tiejun