Re: [Xen-devel] [PATCH 16/22] vixen: pass grant table operations through to the outer Xen

[Anthony Liguori <anthony@xxxxxxxxxxxxx>]

On Sun, Jan 7, 2018 at 9:09 AM, Anthony Liguori <anthony@xxxxxxxxxxxxx> wrote:
> On Sun, Jan 7, 2018 at 8:45 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> wrote:
>> On 07/01/2018 15:42, Anthony Liguori wrote:
>>> On Sun, Jan 7, 2018 at 12:36 AM, Roger Pau Monné <roger.pau@xxxxxxxxxx> 
>>> wrote:
>>>> On Sat, Jan 06, 2018 at 02:54:31PM -0800, Anthony Liguori wrote:
>>>>>  static long
>>>>> +vixen_gnttab_setup_table(
>>>>> +    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>>>> +{
>>>>> +    long rc;
>>>>> +
>>>>> +    struct gnttab_setup_table op;
>>>>> +    xen_pfn_t *frame_list = NULL;
>>>>> +    static void *grant_table;
>>>>> +    XEN_GUEST_HANDLE(xen_pfn_t) old_frame_list;
>>>>> +
>>>>> +    if ( count != 1 )
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if ( unlikely(copy_from_guest(&op, uop, 1) != 0) )
>>>>> +    {
>>>>> +        gdprintk(XENLOG_INFO, "Fault while reading 
>>>>> gnttab_setup_table_t.\n");
>>>>> +        return -EFAULT;
>>>>> +    }
>>>>> +
>>>>> +    if ( grant_table == NULL ) {
>>>>> +        struct xen_add_to_physmap xatp;
>>>>> +        struct domain *d;
>>>>> +        int i;
>>>>> +
>>>>> +        for ( i = 0; i < max_grant_frames; i++ )
>>>>> +        {
>>>>> +             grant_table = alloc_xenheap_page();
>>>> This is wasting one memory page, grant table frames don't need to be
>>>> populated.
>>> Well they have to have a valid struct page_info in order for the guest
>>> to map it within its address space.
>>>
>>> Or did you have something else in mind?
>>
>> Mapping of L0 frames into L1 is a giant mess.
>>
>> First of all, some technical facts:
>> 1) Frames which we map from L0 into L1 do not need to replace existing
>> RAM.  We can use any GFNs up to maxphysaddr.
>> 2) Mapped frames should not replace RAM, and particularly not frames in
>> .data or .bss, because of the performance hit from shattered host
>> superpages.
>> 3) Ideally, we'd want to map into entirely unused GFNs, because then we
>> don't have to interfere with what was there before.
>>
>> In Xen, to allow a frame to be used by a guest, we need to set up domain
>> ownership for it.  This requires a struct page_info to exist, which by
>> default only occurs for pages L1 Xen things is RAM.
>>
>> There is a completely gross way of dealing with this by faking up L1's
>> E820 map to include a range as RAM, and adding every entry in that range
>> into the badpages list.  This causes L1 Xen to put together page_info's
>> for them, but otherwise ignore their existence.
>
> I'll look at this.  I know it's gross but it's pretty straight forward.
>
>> Off the top of my head, frames needing special attention are:
>> * The special pages, including Xenstore and Console rings.  These are
>> real frames (as opposed to mappings), but live inside an E820 hole from
>> L1's point of view.
>> * Shared info
>> * Grant table/status frames
>> * Vcpuinfo frames
>
> I think you mean the runstate area.  We punch this through in Vixen so
> it doesn't need special handling.
>
>> * Event_fifo (if we care to wire that up, but perhaps its not worth it).
>
> I don't think it's worth it TBH.
>
>> What I started doing in PV-shim (before switching to the SP2 side of
>> things fully) was to hard code these mapping frames immediately after
>> the special pages, which is a horrible but safe (as far as I can tell)
>> way of doing things.
>
> I'll take this path after checking myself.
>
>> Ideally, L1 could work out a safe place to use for mappings (which
>> ideally, would be a block of GFNs immediately above the last used
>> frame), but this cannot be done with the toolstack-provided E820 alone,
>> because it is insufficiently descriptive as it deliberately omits
>> information which can be found in the DSDT (e.g. ACPI hotplug regions).
>>
>> The only reasonable option is for L0 to fully understand the guest
>> physical address, and be able to report the details fully to L1,
>> probably in an E820-like way but with our own type identifiers to cover
>> the options which aren't in the E820 spec.
>>
>> This allows L1 to be positively told information such as "This range is
>> safe for mapping into", without having to go and parse all the secondary
>> layout information which is derived from this information in the first
>> place.  Having said that, this will require hypervisor and toolstack
>> changes, so isn't reasonable to retrofit.
>
> Right.  With the current series, no changes are needed to the hypervisor or
> toolstack which is pretty powerful.  Maintaining that property is pretty 
> useful.
>
>> Overall I want to ensure that, whatever plan we come up with for the
>> shim, it doesn't further tangle things up and make them harder to untangle.
>
> For sure.  I think perhaps codifying the HVM/PVH ABI to say that the special
> pages region is, well, special and describing it a bit more is a nice way to
> keep things simple but also make it less of a hack.

Silly me, it already is.  It's the reserved_mem_pgstart field in
hvm_info_table in ACPI.

Regards,

Anthony Liguori

> Regards,
>
> Anthony Liguori
>
>>
>> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel