On 16.10.18 10:37, David Hildenbrand wrote:
> On 19/09/2018 10:47, Janosch Frank wrote:
>> For the upcoming support of VSIE guests on huge page backed hosts, we
>> need to be able to read from large segments.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
>> arch/s390/mm/gmap.c | 43 ++++++++++++++++++++++++++-----------------
>> 1 file changed, 26 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 26cc6ce19afb..ba0425f1c2c0 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -1274,35 +1274,44 @@ EXPORT_SYMBOL_GPL(gmap_mprotect_notify);
>> int gmap_read_table(struct gmap *gmap, unsigned long gaddr, unsigned long *val)
>> {
>> unsigned long address, vmaddr;
>> - spinlock_t *ptl;
>> + spinlock_t *ptl_pmd = NULL, *ptl_pte = NULL;
>> + pmd_t *pmdp;
>> pte_t *ptep, pte;
>> int rc;
>>
>> - if (gmap_is_shadow(gmap))
>> - return -EINVAL;
>> + BUG_ON(gmap_is_shadow(gmap));
>
> Why this change? This is documented behavior (and this is an exported
> function - I think we should keep performing validity checks on exported
> functions).
Sure, I can drop the change.
>
>>
>> while (1) {
>> rc = -EAGAIN;
>> - ptep = gmap_pte_op_walk(gmap, gaddr, &ptl);
>> - if (ptep) {
>> - pte = *ptep;
>> - if (pte_present(pte) && (pte_val(pte) & _PAGE_READ)) {
>> - address = pte_val(pte) & PAGE_MASK;
>> - address += gaddr & ~PAGE_MASK;
>> + vmaddr = __gmap_translate(gmap, gaddr);
>> + if (IS_ERR_VALUE(vmaddr))
>> + return vmaddr;
>> + pmdp = gmap_pmd_op_walk(gmap, gaddr, vmaddr, &ptl_pmd);
>> + if (pmdp && !(pmd_val(*pmdp) & _SEGMENT_ENTRY_INVALID)) {
>> + if (!pmd_large(*pmdp)) {
>> + ptep = gmap_pte_from_pmd(gmap, pmdp, vmaddr, &ptl_pte);
>> + if (ptep) {
>> + pte = *ptep;
>> + if (pte_present(pte) && (pte_val(pte) & _PAGE_READ)) {
>> + address = pte_val(pte) & PAGE_MASK;
>> + address += gaddr & ~PAGE_MASK;
>> + *val = *(unsigned long *) address;
>> + pte_val(*ptep) |= _PAGE_YOUNG;
>> + /* Do *NOT* clear the _PAGE_INVALID bit! */
>> + rc = 0;
>> + }
>> + }
>> + gmap_pte_op_end(ptl_pte);
>
> I'm confused that we have a gmap_pte_op_end() followed by a
> gmap_pmd_op_end() although we never started a gmap_pte_op_walk() ... I
> assume this is due to gmap_pte_from_pmd() ? We should find better names
> for these functions otherwise this is pure magic.
>
> e.g. gmap_pte_op_walk_pmd() instead of gmap_pte_from_pmd()
Hrm, in my opinion pte_from_pmd is very specific, although it lacks the
op part. How about gmap_pte_op_map, that would be closer to the
pte_alloc/offset_map from the kernel side?
>
> ... and shouldn't "gmap_pte_op_end(ptl_pte)" be inside of the "if(ptep)" ?
It doesn't matter as we check the ptl for NULL in op_end functions.
I have that scheme everywhere where it's nicer to read, like in the gmap
protection functions.
I'll have a look if I can make that consistent either way.
>
>> + } else {
>> + address = pmd_val(*pmdp) & HPAGE_MASK;
>> + address += gaddr & ~HPAGE_MASK;
>> *val = *(unsigned long *) address;
>> - pte_val(*ptep) |= _PAGE_YOUNG;
>> - /* Do *NOT* clear the _PAGE_INVALID bit! */
>> rc = 0;
>> }
>> - gmap_pte_op_end(ptl);
>> + gmap_pmd_op_end(ptl_pmd);
>> }
>> if (!rc)
>> break;
>> - vmaddr = __gmap_translate(gmap, gaddr);
>> - if (IS_ERR_VALUE(vmaddr)) {
>> - rc = vmaddr;
>> - break;
>> - }
>> rc = gmap_fixup(gmap, gaddr, vmaddr, PROT_READ);
>> if (rc)
>> break;
>>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
