Re: [Xen-devel] [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/8/18 17:57, Andrew Cooper wrote:
> On 18/08/14 08:42, Chen, Tiejun wrote:
> > On 2014/8/16 0:29, Jan Beulich wrote:
> > > > > > On 15.08.14 at 11:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> > > > > @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct
> > > > > acpi_rmrr_unit *rmrr)
> > > > >        return 0;
> > > > >    }
> > > > >
> > > > > +/* Record RMRR mapping to ready expose VM. */
> > > > > +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
> > > > > +{
> > > > You absolutely need some protection against calling this function more
> > > > than 128 times, or you need to do dynamic allocation of the storage.
> > > This together with ...
> > >
> > > > > --- a/xen/include/asm-x86/e820.h
> > > > > +++ b/xen/include/asm-x86/e820.h
> > > > > @@ -23,6 +23,8 @@ struct e820map {
> > > > >        struct e820entry map[E820MAX];
> > > > >    };
> > > > >
> > > > > +typedef struct e820map rmrr_maps_t;
> > > > This type is a single map of RMRR regions, not multiple maps.
> > > > rmrr_map_t please.
> > > ... this once again stresses what I stated previously: Piggybacking
> > > on the E820 handling here is just the wrong approach. There's
> > > really no correlation with E820 other than us wanting to use the
> > > gathered information for (among other things) adjusting the guest
> > > E820 table. But that doesn't in any way require any re-use of
> > > non-suitable data structures.
> > Why are you saying this is not suitable?
> >
> > We need a structure to represent a RMRR entry including three fields,
> > start, size and type, and especially, essentially RMRR entry belongs
> > to e820 table as one entry.
> Not in Xen.  Only as reported to guests, in which case an e820-like
> structure is most appropriate.
>
> > > In fact I don't see the need for this first patch anyway, as RMRRs
> > > are already being put on a linked list as they get found. I.e. the
> > Yes, that list, acpi_rmrr_unit, can be exposed here. But before you
> > copy to guest, don't you need to grab those fields from that list then
> > convert them as a suitable structure (mostly this is still same as
> > e820entry) to be copied into a buffer?
> Yes, but the hypercall handler can do this which avoids all need to
> store an intermediate representation in Xen.
>
> list_for_each_entry(rmrr, &acpi_rmrr_units, list)
> {
>      e820entry e;
>
>      e.start = ...
>
>      copy_to_guest_offset(...
> }
This is just what I mean. You always need to grab all info from 
acpi_rmrr_units to covert as a e820entry to copy.
So I guess you guys mean we should avoid duplicating RMRR many times in 
global. acpi_rmrr_units is fine enough, so I'd like to do as you expect.
Thanks
Tiejun

> But with appropriate error checking.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel