Re: [Xen-devel] [PATCH] x86/shadow: account for ioreq server pages before complaining about not found mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.04.16 at 14:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/04/16 09:13, Jan Beulich wrote:
>> prepare_ring_for_helper(), just like share_xen_page_with_guest(),
>> takes a write reference on the page, and hence should similarly be
>> accounted for when determining whether to log a complaint.
>>
>> This requires using recursive locking for the ioreq server lock, as the
>> offending invocation of sh_remove_all_mappings() is down the call stack
>> from hvm_set_ioreq_server_state(). (While not strictly needed to be
>> done in all other instances too, convert all of them for consistency.)
>>
>> At once improve the usefulness of the shadow error message: Log all
>> values involved in triggering it as well as the GFN (to aid
>> understanding which guest page it is that there is a problem with - in
>> cases like the one here the GFN is invariant across invocations, while
>> the MFN obviously can [and will] vary).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> IMO, this is a 4.7 candidate. I already had a TODO list item of working
> out why the shadow code was always complaining.

Definitely (hence I had copied Wei).

> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, albeit with one
> further suggestion.
> 
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -35,6 +35,7 @@
>>  #include <asm/current.h>
>>  #include <asm/flushtlb.h>
>>  #include <asm/shadow.h>
>> +#include <asm/hvm/ioreq.h>
>>  #include <xen/numa.h>
>>  #include "private.h"
>>  
>> @@ -2591,7 +2592,8 @@ int sh_remove_write_access_from_sl1p(str
>>  /* Remove all mappings of a guest frame from the shadow tables.
>>   * Returns non-zero if we need to flush TLBs. */
>>  
>> -static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn)
>> +static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn,
>> +                                  unsigned long gfn)
> 
> It would be nice if we could use gfn_t rather than having more unsigned
> longs.

I generally agree, but intentionally didn't to match all the other
shadow code. I'll make changing this dependent on what Tim
thinks.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel