Re: [PATCH 2/3] xen-fbfront: Use grant references when requested

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 01/07/2011 04:02 PM, Daniel De Graaf wrote:
> On 01/07/2011 03:44 PM, Ian Campbell wrote:
>> On Fri, 2011-01-07 at 16:12 +0000, dgdegra@tycho.nsa.gov wrote: 
>>> @@ -556,17 +552,58 @@ static unsigned long vmalloc_to_mfn(void *address)
>>>  	return pfn_to_mfn(vmalloc_to_pfn(address));
>>>  }
>>>  
>>> -static void xenfb_init_shared_page(struct xenfb_info *info,
>>> +static void xenfb_init_shared_page(struct xenbus_device *dev,
>>> +                                   struct xenfb_info *info,
>>>  				   struct fb_info *fb_info)
>>>  {
>>> -	int i;
>>>  	int epd = PAGE_SIZE / sizeof(info->mfns[0]);
>>> +	int be_id = dev->otherend_id;
>>> +	int i, ref;
>>> +	unsigned long mfn;
>>> +	grant_ref_t gref_head;
>>> +	int allpages = info->nr_pages + ((info->nr_pages + epd - 1) / epd) + 1;
>>> +
>>> +	int grants = 0;
>>> +	xenbus_scanf(XBT_NIL, dev->otherend, "use-grants", "%d", &grants);
>>
>> There doesn't seem to be any negotiation with the backend about whether
>> or not grants should be used so there is no way for a backend to know if
>> it can choose to set this flag or not, granted not all backends will
>> have a choice due to their privilege level.
> 
> The "use-grants" flag is a request by the backend; older versions will ignore
> it, newer versions (may) honor it.
>  
>> More importantly there is also no way for the backend to figure out is
>> the frontend is going to obey the request if it does write it (at least
>> until it tries to map a gref and fails because its really got an mfn).
> 
> The key used in xenstore (page-gref versus page-ref) tells the backend if
> the request was honored. If the backend is unable to map MFNs, then it will
> fail if page-gref is not present; otherwise, it can fall back to the old
> MFN mapping.
>  
>> Usually both front and backend would write a feature-foo node to their
>> respective directory in xenstore and then figure out what to do based on
>> what the other end wrote.
> 
> Perhaps "use-grants" should be renamed to "feature-grants"? I thought the
> frontend writing a feature-grants node would be redundant since the
> page-gref node already communicated what was chosen.
>  
>> In the kbdfront patch you simply write both the mfn and the grant
>> reference to xenstore, presumably the backend then just picks for itself
>> which access method to use, could that approach be applicable here?
> 
> Possible, but it would waste a bit of memory (a few extra pages for the
> MFN tables in addition to the grant-ref tables). If this isn't an issue,
> then fbfront can expose both and allow the backend to choose without any
> negotiation. Is this preferred?
>  
>> There seems to be slack in xenfb_page which could accommodate a second
>> pd array containing grefs for the pages. The presence of a page-gref
>> node in xenstore would indicate that the larger structure with the grefs
>> is in use.
>>
>> Ian.
>>
> 
> Yes, there is space in xenfb_page, since grants are always 32-bit numbers.
> The offset will be different depending on sizeof(unsigned long) in the guest,
> due to bad design of the shared page, but that's not hard to work around.
> Alternatively, we could put in some padding to fix the offset.
> 

Actually, after looking at xenfb_page again, there isn't room, and the current
structure in the linux kernel is very misleading. Offset 1024 in the structure
is the start of the incoming ring buffer, and offset 2048 is the outgoing ring
buffer. The existing pd array overlaps both of these ring buffers (on 64-bit;
on 32-bit it only overlaps the incoming ring).

-- 
Daniel De Graaf
National Security Agency