Re: inconsistent pfn type checking in process_page_data

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/10/2020 17:41, Olaf Hering wrote:
> Andrew,
>
> with commit 93c2ff78adcadbe0f8bda57eeb30b1414c966324 a new function 
> process_page_data was added. While filling the mfns array for 
> xenforeignmemory_map, the individual pfn types[] are checked in a different 
> way than the checking of the result of the mapping attempt. 
>
> Is there a special reason why the first loop uses the various TAB values, and 
> the other loop checks XTAB/BROKEN/XALLOC? The sending side also uses the 
> latter.

Hmm.  (Wow that's more than 5 years old now)

IIRC, that particular piece of logic took an age to get correct.  Its
quite possible it ended up being a merge of code which David and I wrote
at slightly different times.

Fundamentally, there are 4 bits of page type which encode NOTAB
(writeable), L{1..4}, Pinned L{1..4}, XTAB (invalid), XALLOC
(allocate-only - still not sure what the purpose is.  There is no
producer of this type that I'm aware of) and BROKEN.

By my count, that is 12 legal types, so asymmetric type=>"is there data"
conversions is probably a bad thing.

I suspect we probably want an is_known_page_type() predicate on the
source side to sanity check data from Xen, and on the destination side
in handle_page_data() to sanity check data in the stream, and we
probably want a page_type_has_data() predicate to use in
write_batch()/process_page_data() to ensure that the handling is consistent.

Do you fancy doing a patch?  If not I can put this on my TODO list, but
no ETA on when I'd get to it.

~Andrew

Attachment: signature.asc
Description: OpenPGP digital signature